Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes behavior of --no-* options #795

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

usmonster
Copy link
Contributor

@usmonster usmonster commented May 2, 2018

A bug existed whenever a --no-prefixed option was defined along with
its counterpart (e.g. --status and --no-status), which caused the
corresponding boolean attribute to always be false when either option
was specified on the command line.

This was because the definition of a negating option would overwrite the
default value of the attribute, combined with the fact that specifying either
option would emit two events with conflicting logic.

This has been corrected in two ways:

  1. During the initialization of a negating option, we now check to see if the
    non-negated counterpart was already defined, in which case we don't
    set/overwrite the default value (which would otherwise be set to true);
  2. An option's name no longer omits its negation prefix; that is,
    "--no-cheese" is now internally named "no-cheese", which will distinguish
    it from "--cheese". This will allow unique listeners & events to be registered and
    emitted, depending on which option is passed to the command, thus
    avoiding any attribute assignment collision/race.

Additionally, the tests for negated options were updated to more
explicitly demonstrate the expected behavior, and a couple of relevant
examples were fixed to match their intended behavior.

return this.long
.replace('--', '')
.replace('no-', '');
return this.long.replace(/^--/, '');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This may be a breaking change for people who use the (only recently-documented) Custom Event Listeners, though I can also clarify this by adding another illustrative example to the Readme in that section, if required.

@usmonster
Copy link
Contributor Author

Bump–@vanesyan, does this look OK for you?

@roman-vanesyan
Copy link
Collaborator

Yes, everything is ok for me, but cannot merge this until the next major bump as it's breaking change.

@shadowspawn
Copy link
Collaborator

Good description of problem and changes, thanks @usmonster

I am hopeful this will tidy up --no- options! I have done a visual inspection of changes. I have not tested runtime yet.

Also good catch that the JSDoc was claiming default value was true rather than undefined, oops.

@shadowspawn
Copy link
Collaborator

@usmonster
Thanks for updating PR, and comments in #939.

The change to the custom event listener makes this a breaking change for a major version, so I have been working on some other pull requests first.

Good to know you are still around and interested, makes this one high on my list. :-)

@shadowspawn
Copy link
Collaborator

(In top two of my PR queue. Hopefully take another look this week.)

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jun 2, 2019

I have been trying all the permutations by running programs to check behaviour, as the code itself is quite subtle. One request, one comment, one thing I am still wondering about, one future change. (Sorry, I do some long comments!)

Overall, I think this will make --no- useful and usable in combinations, and worth the pain of a breaking change. Thanks! I think the original vision was just for a stand-alone negated boolean, but people quite reasonably want and try to use it in combination with all forms of options. And are disappointed.

  1. Request: I think in practice the option with a required value (e.g. <value>) should be people's first choice, and the option with optional value (e.g. [value]) is less useful and harder to handle. So for the pizza example, I would like cheese to be required rather than optional. (But the existing example and output are inconsistent, well spotted!)

  2. Musing: I am wondering about name in opts. This is the only use of operator in and stuck out to me. Would testing .optionFor be appropriate and more consistent with existing code?

  3. Breaking note. This is a breaking change to the default values of options when no command line arguments are supplied (and other situations, but this is the big one). I think the new behaviour is a reasonable one to explain, simple story: declaring --no-foo after --foo does not change default value of foo.

const program = require("commander");

program
  .option('-c, --cheese <type>', 'Add the specified type of cheese', "marble")
  .option('-C, --no-cheese', 'You do not want any cheese')
  .option('--what <type>')
  .option('--no-what');

program.parse(process.argv);
console.log(`cheese is ${program.cheese}`);
console.log(`what is ${program.what}`);
# Before:
$ node required.js
cheese is true
what is true

# After:
$ node required.js
cheese is marble
what is undefined
  1. Future: I think we should allow a default value for pure flags. This would allow an externally supplied default which can be overridden on command line. Fixing up --no-* is first though and better to deal with one thing at a time, so I am not suggesting you do this in this PR, but it was on my mind.
program
  .option('--what <type>', 'enable on command line', process.env.WHAT)
  .option('--no-what', 'disable on command line');

@slavafomin
Copy link

Please, take a look at my comment here.

@usmonster
Copy link
Contributor Author

usmonster commented Jun 23, 2019

Thanks for the comment, @slavafomin. I replied in the thread.

Belated thanks, @shadowspawn, for the thorough review and feedback! No need to apologize for long comments, as they are appreciated. :) I'll reply to each below:

  1. Totally agree, updated the example!
  2. I'm all for consistency, but .optionFor doesn't really work in a way that makes it useful in this case. I think name in opts is exactly the right tool for the job in this case, so I'll leave it as-is, if you don't mind?
  3. Just to clarify, would you like me to document a new example in the readme, or update my commit message to include your example, or is it enough to just add a comment to the pizza example with the new expected output? Personally, I think the changelog/release notes should be enough if you prefix the commit subject with [BC BREAK], but OK for me if you also want something more explicitly documenting the new behavior.
  4. I understand what you mean, and I read through Support defaults for boolean options (including --no-*), for env var set defaults #928 to see the use cases, but I'm still not convinced about its utility. Based on the example you gave in the above comment, I think there's already a pretty straightforward way to do that:
const handleWhat = function() {
  // negate `what` if default isn't `true`
  if ('WHAT' in process.env && process.env.WHAT !== 'true') {
    this.what = !this.what;
  }
};

program
  .option('--what', 'enable on command line')
  .option('--no-what', 'disable on command line')
  .on('option:what', handleWhat)
  .on('option:no-what', handleWhat)
  .parse(process.argv);

console.log(`what is ${program.what}`);

It can even be done without event listeners by doing the conditional reassignment at some point after the .parse (example in my comment). In any case, if enough people want this to be a built-in feature, I'll let someone else make that PR, but I'm kinda not for it. ;)

Thanks in advance for your feedback, especially on my question in point 3. :)

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jun 24, 2019

  1. Thanks. :-)

  2. What about {}.hasOwnProperty.call(opts, name)instead then? I am paranoid about (more) conflicts with built-in properties. (I have not tested runtime, suggesting this in theory.)

e.g. https://eslint.org/docs/rules/guard-for-in

  1. No changes from you needed thanks. I was thinking out loud and making a note to myself, it isn't just the event listener change that is breaking. I'll update the README section on options and comfortable doing that myself after carefully crafting the current coverage.

  2. I have the ball with that one. My example was not very good. I might invite you to review if I get a candidate Pull Request done for v3. 😄

index.js Outdated Show resolved Hide resolved
A bug existed whenever a "--no"-prefixed option was defined along with
its counterpart (e.g. "--status" and "--no-status"), which caused the
corresponding boolean attribute to always be `false` when either option
was specified on the command line.

This was because the definition of a negating option would overwrite the
default value of the attribute, combined with the fact that specifying either
option would emit two events with conflicting logic.

This has been corrected in two ways:
1. During the setup of a negating option, we now check to see if the
   non-negated counterpart was already defined, in which case we don't
   set/overwrite the default value (which would be "true");
2. An option's name no longer omits its negation prefix; that is,
   "--no-cheese" is internally "no-cheese", which will distinguish it
   from "--cheese"—this will allow unique events to be registered and
   emitted, depending on which option is passed to the command, thus
   avoiding any attribute assignment collision.

Additionally, tests for negated options were added/updated to more
explicitly demonstrate the expected behavior, and a couple of relevant
examples were fixed to match their intended behavior.
@usmonster usmonster changed the base branch from master to release/3.0.0 June 24, 2019 13:31
@shadowspawn
Copy link
Collaborator

Thanks for switching base branch! I have not been asking people to do that in case it creates problems, but does make it easier for me. :-)

@shadowspawn
Copy link
Collaborator

Starting merge.

@shadowspawn shadowspawn merged commit 26594fb into tj:release/3.0.0 Jun 27, 2019
@shadowspawn
Copy link
Collaborator

Updated example and README with new improved behaviour, and added entries to CHANGELOG

This is one of my favourite changes in v3. 😄

Thank you for your contributions.

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jun 27, 2019
@usmonster
Copy link
Contributor Author

Thanks for your proactive involvement!

@shadowspawn
Copy link
Collaborator

Available now as a prerelease. See #1001

@shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants